Skip to content

LSPS5 implementation #3662

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

martinsaposnic
Copy link
Contributor

@martinsaposnic martinsaposnic commented Mar 11, 2025

A complete implementation for LSPS5 (spec defined here lightning/blips#55)

Reviewing commit by commit is recommended (~40% of the added lines are tests)

Notes:

@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Mar 11, 2025

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

@martinsaposnic
Copy link
Contributor Author

martinsaposnic commented Mar 11, 2025

This is a huge PR, but it wasn’t obvious to me how to split it in a way that would still make sense (I did split it into small commits to make it easier to review.). I’m open to suggestions if you have ideas on how this could be structured differently.

Copy link

codecov bot commented Mar 11, 2025

Codecov Report

Attention: Patch coverage is 86.38596% with 194 lines in your changes missing coverage. Please review.

Project coverage is 88.84%. Comparing base (2f9898c) to head (2586494).

Files with missing lines Patch % Lines
lightning-liquidity/src/lsps5/msgs.rs 76.84% 81 Missing and 16 partials ⚠️
lightning-liquidity/src/lsps5/client.rs 90.59% 30 Missing and 3 partials ⚠️
lightning-liquidity/src/lsps5/service.rs 91.16% 20 Missing and 5 partials ⚠️
lightning-liquidity/src/lsps0/ser.rs 79.16% 3 Missing and 17 partials ⚠️
lightning-liquidity/src/lsps5/url_utils.rs 94.87% 1 Missing and 5 partials ⚠️
lightning-liquidity/src/manager.rs 93.84% 2 Missing and 2 partials ⚠️
lightning-liquidity/src/lsps5/validator.rs 95.83% 0 Missing and 3 partials ⚠️
lightning/src/util/test_utils.rs 50.00% 3 Missing ⚠️
lightning/src/ln/blinded_payment_tests.rs 0.00% 2 Missing ⚠️
lightning-liquidity/src/lsps0/msgs.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3662      +/-   ##
==========================================
- Coverage   89.05%   88.84%   -0.21%     
==========================================
  Files         167      171       +4     
  Lines      121800   123097    +1297     
  Branches   121800   123097    +1297     
==========================================
+ Hits       108464   109364     +900     
- Misses      10928    11351     +423     
+ Partials     2408     2382      -26     
Flag Coverage Δ
fuzz ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ldk-reviews-bot ldk-reviews-bot requested a review from arik-so March 11, 2025 20:22
@tnull tnull self-requested a review March 11, 2025 20:37
@wpaulino wpaulino removed the request for review from arik-so March 11, 2025 20:39
Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, thank you for looking into this! I did a first pass, and it looks pretty amazing already!

Before going too much into further details, here are a few general comments upfront:

  1. I'm generally no fan of introducing additional dependencies here, an in particular not reqwest and tokio. I think following the pattern so far BroadcastNotifications could be a request that the user handles with any HTTP client they want and then could call back into LSPS5ServiceHandler. Alternatively, we could also use a trait similar to the current HTTPClient, but I don't think we want to keep the default implementation. Note that the blocking reqwest variant wraps a tokio runtime internally, and therefore should never (1, 2, ...) be used together. I guess technically we could consider a default async version of the trait that uses async reqwest, but I would prefer to simply have well-documented trait on our end that the user can implement however they choose to. Also note that stacking tokio runtimes is heavily discouraged in general, so assuming our users would themselves use a tokio runtime, we shouldn't wrap one in LSPS5ServiceHandler.

  2. Note that lightning-liquidity is optionally no-std compliant, so please don't rely on std wherever possible, often it's just a matter of using core instead and importing the respective types from crate::prelude. If you really find yourselves needing to use std, make sure it's feature gated behind feature = "std" and we provide an alternative for users that don't support it.

  3. Minor: Regarding formatting we're using tabs, not spaces. Feel free to run ./contrib/run-rustfmt.sh after each commit to run our formatting scripts.

  4. This PR in its current scope is great, just want to note that eventually we need to add persistence for the state. As we haven't fully fleshed out the persistence strategy for lightning-liquidity in general yet, it's actually preferred to defer this to a follow-up, but just wanted to mention it. Also note that some changes to MessageQueue/EventQueue will happen in #3509, but will ping you to rebase once that has been merged. (just sidenotes here).

I hope these initial points make sense, let me know if you have any questions, or once you made corresponding changes and think this is ready for the next round of review!

@ldk-reviews-bot
Copy link

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

@tnull
Copy link
Contributor

tnull commented Mar 13, 2025

This is a huge PR, but it wasn’t obvious to me how to split it in a way that would still make sense (I did split it into small commits to make it easier to review.). I’m open to suggestions if you have ideas on how this could be structured differently.

Thanks for asking! I'm totally fine to keep this (with its current scope) in a single PR, as long as we keep the commit history pretty clean to allow continuing review to happen commit-by-commit. To this end, please make sure add any fixup commits clearly marked (e.g. via a f or fixup prefix in the commit message header) directly under the commit they belong to, so they can cleanly be squashed into the respective feature commits in-between review rounds.

@tnull
Copy link
Contributor

tnull commented Mar 13, 2025

Btw, I'm not sure if you're familiar with the previous attempt of implementing LSPS5: #3499

Given this is a clean slate, not sure how much there is to learn, but still might be worth a look. Also not sure if @johncantrell97 would be interested in reviewing this PR, too, as he's familiar with the codebase and LSPS5.

@martinsaposnic martinsaposnic marked this pull request as draft March 14, 2025 14:49
@martinsaposnic martinsaposnic force-pushed the lsps5 branch 2 times, most recently from 1d4b47c to edf5346 Compare March 14, 2025 16:31
@martinsaposnic martinsaposnic marked this pull request as ready for review March 14, 2025 16:31
@martinsaposnic
Copy link
Contributor Author

martinsaposnic commented Mar 14, 2025

@tnull, ready for the next review round!

  • Removed reqwest and tokio.
  • HttpClient is now a generic trait, allowing users to pass any implementation.
  • Removed std.
  • Small refactor on client and service to be able to delete some silly / unnecessary code.
  • Ran formatting on every commit.
  • All new changes are in commits prefixed with fixup:.

CI is failing because of the usage of the url crate (which I guess does not support rust 1.63?), which I need for validating the webhook URL. A new commit will come addressing that shortly

@martinsaposnic martinsaposnic requested a review from tnull March 14, 2025 17:12
@martinsaposnic martinsaposnic force-pushed the lsps5 branch 7 times, most recently from 9809682 to af5929e Compare March 16, 2025 14:24
@ldk-reviews-bot
Copy link

🔔 1st Reminder

Hey @tnull @valentinewallace! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

1 similar comment
@ldk-reviews-bot
Copy link

🔔 1st Reminder

Hey @tnull @valentinewallace! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@martinsaposnic martinsaposnic force-pushed the lsps5 branch 2 times, most recently from 2586494 to d0f64cc Compare July 19, 2025 19:37
@martinsaposnic
Copy link
Contributor Author

Although, now that #3927 landed, this PR unfortunately needs a rebase and the tests have to to be re-written based on the test_utils (either way, but I now went with landing #3927 as it was ready, hope that's okay).

Rebase done and conflicts resolved.

Also, the bug discussed on friday (notification signing flow bug) is fixed now. As discussed, a sign_message method is added to NodeSigner, and a NodeSigner is added to LiquidityManager and to LSPS5/service so it can sign notifications correctly. Relevant commits: 0e8a683 and 7ff724d

I agree we can avoid the timestamp validation, and probably can drop TimeProvider from LSPS5ClientHandler and LSPS5Validator. I'm actually also tempted to drop the spec protocol extensibility (i.e., to use anything else but HTTPS) and the x-lsps5-timestamp from the spec.

Happy to have these changes happen here or in a follow-up.

This sounds good to me, and I have the changes ready (in a different branch), which I prefer to include in a follow up once this PR is merged, if that's ok to you @tnull @TheBlueMatt

Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixups look mostly good to me. I did another pass and commented what stood out to me. For me only the comments regarding the new NodeSigner::sign_message interface and revisiting the commit messages are blocking, everything else could happily be done in follow ups after we land this.

config.clone(),
time_provider,
);
time_provider.clone(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we suddenly need to clone here?

@@ -930,6 +930,9 @@ pub trait NodeSigner {
/// message to be broadcast, as otherwise it may prevent one from receiving funds over the
/// corresponding channel.
fn sign_gossip_message(&self, msg: UnsignedGossipMessage) -> Result<Signature, ()>;

/// Sign an arbitrary message with the node's secret key.
fn sign_message(&self, msg: &[u8]) -> Result<String, ()>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should be able to make this infallible? Should we include the docs from sign here, too?

Also, I wonder if we also mention that this might panic, similar to what we do above in sign_gossip_message, although it seems we could do a better job describing when this would panic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will make this only return String.

I thought it made sense to keep the style of the other sign methods, but message_signing::sign does not panic, so I don't see the point of returning a Result instead of a String

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixup commit a1180a6, also improving the docs on the sign_message method @tnull

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, the fixup looks good, but I have to eat my words as it turns out we made the signing methods fallible to account for the case of a remote signer. So, we need to make sign_message fallible afterall, excuse the churn :/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem at all. I dropped the commit that changed the sign_message method return type and replaced it with this one 75ece86 that only changes the documentation of sign_message

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Commit message of this commit is outdated by now (e.g., we don't have an HttpClient trait anymore). Might be good to revisit all of them once more to check they reflect the current state before we land this PR?

///
/// This trait is used to provide the current time for LSPS5 service operations
/// and to convert between timestamps and durations.
pub trait TimeProvider {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth moving this to some crate::utils::time or similar path, if we'll end up keeping it at all. (in a follow-up)

self.broadcast_notification(client_id, notification)
}

fn broadcast_notification(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: This is named a bit misleading, as we just send the notification, we don't 'broadcast' (i.e., disseminate to multiple peers/endpoints). (could be renamed in a follow-up)

if webhook
.last_notification_sent
.get(&notification.method)
.map(|last_sent| now.clone().abs_diff(&last_sent))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should just make LSPSDateTime copy rather than explictly cloneing here and below, etc.

(in a follow-up)

@@ -214,6 +239,16 @@ impl LSPSDateTime {
self.0.timestamp().try_into().expect("expiration to be ahead of unix epoch");
now_seconds_since_epoch > datetime_seconds_since_epoch
}

/// Returns the time in seconds since the unix epoch.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These docs are wrong. I also wonder if this should just be fn duration_since(&self, other: &Self) -> Duration rather than returning a u64. (could both be done in a follow-up)

#[derive(Debug, Clone)]
/// Configuration for the LSPS5 client
pub struct LSPS5ClientConfig {
/// Maximum age in seconds for cached responses (default: 3600 - 1 hour).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Rather than replicating the default values in the doc, let's just link to DEFAULT_RESPONSE_MAX_AGE_SECS, which avoids them getting stale (in a follow-up).

{
let mut outer_state_lock = self.per_peer_state.write().unwrap();
let inner_state_lock = outer_state_lock.entry(counterparty_node_id).or_insert(Mutex::new(
PeerState::new(self.config.response_max_age_secs, self.time_provider.clone()),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll cleanup the individual response entries, but would we ever clean up the overall PeerState once it's empty?

This will be used to allow disabling time-dependent functionality.
'time' is included in the features default.
Introduce new_from_duration_since_epoch constructor from Duration. Also
add abs_diff function to use on client / service. Also do feature='time'
instead of feature='std' on time related functionality
@martinsaposnic
Copy link
Contributor Author

martinsaposnic commented Jul 21, 2025

The requested changes on NodeSigner::sign_message are pushed 51a0655, and all the commit messages were reviewed and rewritten accordingly.

Let me know if they look ok so I can squash them @tnull

Changes coming in a follow up (I may create an issue for this?):

@tnull
Copy link
Contributor

tnull commented Jul 21, 2025

The requested changes on NodeSigner::sign_message are pushed 51a0655, and all the commit messages were reviewed and rewritten accordingly.

Let me know if they look ok so I can squash them @tnull

Changes coming in a follow up (I may create an issue for this?):

* [ ]  avoid timestamp validation on validator

* [ ]  drop TimeProvider from LSPS5ClientHandler and LSPS5Validator

* [ ]  check unnecesary time_provider.clone() in manager [LSPS5 implementation #3662 (comment)](https://github.com/lightningdevkit/rust-lightning/pull/3662#discussion_r2218510341)

* [ ]  Move time_provider trait into crate::utils::time or similar [LSPS5 implementation #3662 (comment)](https://github.com/lightningdevkit/rust-lightning/pull/3662#discussion_r2218559905)

* [ ]  rename broadcast_notification in service ([LSPS5 implementation #3662 (comment)](https://github.com/lightningdevkit/rust-lightning/pull/3662#discussion_r2218580234))

* [ ]  copy LSPSDateTime instead of cloning [LSPS5 implementation #3662 (comment)](https://github.com/lightningdevkit/rust-lightning/pull/3662#discussion_r2218590430)

* [ ]  docs wrong and return Duration instead of u64 on abs_diff [LSPS5 implementation #3662 (comment)](https://github.com/lightningdevkit/rust-lightning/pull/3662#discussion_r2218598607)

* [ ]  do not duplicate DEFAULT_RESPONSE_MAX_AGE_SECS [LSPS5 implementation #3662 (comment)](https://github.com/lightningdevkit/rust-lightning/pull/3662#discussion_r2218624527)

* [ ]  clean up the overall PeerState once it's empty [LSPS5 implementation #3662 (comment)](https://github.com/lightningdevkit/rust-lightning/pull/3662#discussion_r2218635296)

Yes, please open (a) tracking issue(s) for these, preferably with links to the original comment to establish context.

Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, feel free to squash from my side, and then we should land this as-is, IMO, if that's alright with @TheBlueMatt

Allows validating webhook URLs without depending on the external url crate.
Define LSPSMessage request, response and error types.
Also introduce LSPS5AppName, LSPS5WebhookUrl, WebhookNotification types
Introduce SendWebhookNotification event for LSPS5/service, and also
WebhookRegistered, WebhookRegistrationFailed, WebhooksListed,
WebhookRemoved and WebhookRemovalFailed events for LSPS5/client.
Implements the LSPS5 webhook registration service that
allows LSPs to notify clients of important events via webhooks.
This service handles webhook registration, listing, removal,
and notification delivery according to the LSPS5 specification.
Implements the client-side functionality for LSPS5 webhook registration,
allowing clients to register, list, and remove webhooks with LSPs.
…ions from an LSP.

As context, this utility started as part of the client, but was extracted
in favor of having it separated, so it's clear that this functions should not
be used by the client, but by the proxy server that has to forward the notifications
Fully integrates the LSPS5 webhook components into the
lightning-liquidity framework, enabling usage through the LiquidityManager.
@martinsaposnic
Copy link
Contributor Author

Fixups squashed!

issue with follow ups #3944

@@ -930,6 +930,14 @@ pub trait NodeSigner {
/// message to be broadcast, as otherwise it may prevent one from receiving funds over the
/// corresponding channel.
fn sign_gossip_message(&self, msg: UnsignedGossipMessage) -> Result<Signature, ()>;

/// Sign an arbitrary message with the node's secret key.
Copy link
Collaborator

@TheBlueMatt TheBlueMatt Jul 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit This makes it sound like the message should be signed directly, but in fact we should sign with the prefix "Lightning Signed Message:"

std = ["lightning/std", "lightning-liquidity/std", "bitcoin-io/std", "bitcoin_hashes/std"]
time = ["std", "lightning-liquidity/time"]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're gonna introduce a time feature in the BP crate we have to make sure we don't fetch time when its not set. That means disabling the threaded/std BP constructor too. I'm not really convinced having a separate feature here is worth it, then, because we enable the same set of things between std and time (there's only really two things in the whole crate - the async+no-std BP and the sync/threaded/std/time one).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants